fix(runtime): respect keepPlaying option in player seek#863
fix(runtime): respect keepPlaying option in player seek#863calcarazgre646 wants to merge 1 commit into
Conversation
The runtime player's seek unconditionally pauses on every invocation, so
A/E Jump-to-in/out shortcuts (which pass { keepPlaying: true } per PR
heygen-com#842) pause playback in compositions backed by the __player runtime
adapter. Only the wrapTimeline path honoured the option.
Extend RuntimePlayer.seek and the PlayerAPI contract to accept
{ keepPlaying?: boolean }. When keepPlaying is set and playback was
active, resume the master plus rearmed sibling timelines and emit the
play-state events so media and analytics stay in sync. Default behaviour
is unchanged when no options are passed.
jrusso1020
left a comment
There was a problem hiding this comment.
Verdict
Approve. Clean, narrow fix that closes the asymmetry left by #842: the __player runtime adapter now honours { keepPlaying: true } so A/E Jump-to-in/out shortcuts preserve playback for window.__player-backed compositions, just like the wrapTimeline (GSAP) path already did. Type contract is propagated end-to-end (PlayerAPI → RuntimePlayer → createPlayerApiCompat basePlayer shape), implementation mirrors the existing play() body, and the 6 new tests cover the meaningful branches including the wasPlaying === false no-op and the explicit keepPlaying: false back-compat case.
Spot-checked claims against source:
useTimelinePlayer.getAdapter()does returnwin.__playerdirectly (no wrapper) —usePlaybackKeyboard.tsA/E handlers do pass{ keepPlaying: true }—wrapTimeline.seekskips its internaltl.pause()whenkeepPlayingis set. All check out.seekMasterAndSiblingTimelinesDeterministicallydoes pause the master and re-pause rearmed siblings, so the new branch correctly has to calltimeline.play()+ iterate siblings to resume — matches the symmetric pattern inplay()(lines 117–129).wasPlayingis captured before the deterministic seek; the helper manipulates timelines but never touchesisPlayingstate, so the read is accurate.
Key Concerns
None blocking.
Test Coverage
Solid. The invocationCallOrder assertion in "resumes the master timeline after the deterministic seek pauses it" is a good way to express "play() must come after the helper's pause()" — that's exactly the regression vector. Coverage:
keepPlaying: true && wasPlaying→ preserves state, emitsonDeterministicPlay+onShowNativeVideos+onSyncMedia(_, true)✓keepPlaying: true && !wasPlaying→ stays paused (intent is preserve, not force) ✓keepPlaying: falseexplicit → default path ✓- No options → default path (back-compat) ✓
- Sibling
timeScale+play()propagation ✓ - Master
play()ordered after the helper'spause()✓
Gap (non-blocking): no studio-side integration test in useTimelinePlayer.seek.test.ts exercising A/E → adapter.seek(_, { keepPlaying: true }) against the runtime adapter. The unit coverage is sufficient given options pass straight through useTimelinePlayer.seek → adapter.seek and the studio test suite passes, but a single end-to-end assertion would catch a future delegate regression.
Nits / Future
-
createStaticSeekPlaybackAdaptertype drift (out of scope, called out in PR). Worth tightening the type now — it's a one-line change and the body already preserves playback by accident. Leaving the type signature mismatched is a footgun for the next person who reads the adapter and thinkskeepPlayingis unhandled. Happy to follow up in a separate PR if you'd rather keep this one tight. -
Sibling play→pause→play churn. During a
keepPlayingseek, each sibling seesplay()(rearm) →pause()(helper finally block) →play()(this branch). Functionally correct, but three GSAP state transitions per seek per sibling. PlumbingkeepPlayingintoseekMasterAndSiblingTimelinesDeterministicallyso it skips the final pause when the caller is going to immediately resume would be cleaner. Optimization, not a bug. -
onDeterministicPauseis never emitted byseek(), regardless of branch. This matches the prior behaviour (the originalseekonly emittedonDeterministicSeek), so no regression — flagging only because theplay/pausemethods do emit their pairedonDeterministicPlay/onDeterministicPause, and an external listener relying on pair-symmetry could be surprised. Probably intentional, but worth being aware of. -
setIsPlaying(true)is not called in the keep-playing branch. Fine becausewasPlaying === truemeans state is already true, but it makes the branch slightly asymmetric toplay()(which always emitssetIsPlaying(true)). Not worth changing — just noting for future readers.
Nice cleanup of the loose end from #842.
Summary
Closes the follow-up I promised in #842 (comment): the
__playerruntime adapter ignored the{ keepPlaying: true }option that A/E Jump-to-in/out shortcuts send, so playback pauses on every press even though the caller asked to preserve it. Only thewrapTimeline(GSAP) path honoured the option.Background
PR #842 added
seek: (time, options?: { keepPlaying?: boolean })toPlaybackAdapterand wired the A/E shortcuts inusePlaybackKeyboardto pass{ keepPlaying: true }. The fix landed correctly for compositions backed by__timeline/__timelines(GSAP), becausewrapTimelinereads the option and skips its internaltl.pause().For compositions backed by
window.__player(packages/core/src/runtime/init.ts:1529),useTimelinePlayer.getAdapter()returns the runtime adapter directly without a wrapper. That adapter'sseekisbasePlayer.seekfromcreateRuntimePlayer(packages/core/src/runtime/player.ts:147), and the implementation was:setIsPlaying(false)runs unconditionally, so A/E pauses the moment the shortcut fires. The option that the Studio sends never reaches the implementation because both the localbasePlayershape increatePlayerApiCompatand the publicPlayerAPI.seek/RuntimePlayer.seektypes only declared(time: number) => void.Fix
Extend the type contract from end to end so the option is preserved through
createPlayerApiCompat:core.types.ts—PlayerAPI.seek(time, options?: { keepPlaying?: boolean })runtime/types.ts—RuntimePlayer.seeksignatureruntime/init.ts:88— localbasePlayershape increatePlayerApiCompatImplement the behaviour in
createRuntimePlayer.seek:wasPlayingbefore the deterministic seek helper pauses the master and rearmed siblings.options.keepPlaying && wasPlaying, resume the master + siblings, reapplyplaybackRateviatimeScale, and emitonDeterministicPlay+onShowNativeVideos+onSyncMedia(t, true)so media and analytics stay in sync.keepPlaying: false, orwasPlaying === false) keeps the existingsetIsPlaying(false)+onSyncMedia(t, false)path. No back-compat change.Test plan
Added 6 new tests in
packages/core/src/runtime/player.test.tsunderdescribe(\"keepPlaying option\"):keepPlaying: trueand playback was active (nosetIsPlaying(false), emitsonDeterministicPlay+onSyncMedia(t, true)).play()is invoked after the last internalpause()viainvocationCallOrder).playbackRateto master and siblings on resume.keepPlaying: truebut playback was not active (intent is preserve, not force).keepPlaying: falsematches default behaviour.Other validations:
bun run --cwd packages/core test→ 868/868 passing (was 862, +6 new).bun run --cwd packages/studio test→ 514/514 passing, no regression in the consuming side.bun run --cwd packages/core typecheckandbun run --cwd packages/studio typecheckclean.bunx oxlintandbunx oxfmt --checkclean on touched files.Out of scope
createStaticSeekPlaybackAdapterinpackages/studio/src/player/lib/playbackAdapter.tsstill hasseek: (time) => void(no options). Its current behaviour happens to preserve playback by accident (theplayingflag drives the RAF ticker independently), but the contract is mismatched. Happy to follow up if a maintainer wants the static-seek adapter brought in line with the same shape.Related
__playerpath concern.